Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

agent: golangci-lint fixes. #425

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

klihub
Copy link
Collaborator

@klihub klihub commented Dec 10, 2024

Fix golangci-lint issues.

Signed-off-by: Krisztian Litkey <[email protected]>
@askervin askervin force-pushed the fixes/golangci-lint/agent branch from b4c9232 to 83c12bd Compare December 11, 2024 06:33
Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

My only concern here is dropping connection timeout. It would definitely be a corner case that the socket exists but kubelet is not responding. I'm not too familiar with grpc, so I don't know if this could lead into a situation where our plugins that use podresapi would stop responding, too, and get kicked out from container runtime.

@klihub, do you think this can cause issues, or do you think plugins will recover just fine when kubelet is alive again?

@klihub
Copy link
Collaborator Author

klihub commented Dec 11, 2024

LGTM.

My only concern here is dropping connection timeout. It would definitely be a corner case that the socket exists but kubelet is not responding. I'm not too familiar with grpc, so I don't know if this could lead into a situation where our plugins that use podresapi would stop responding, too, and get kicked out from container runtime.

@klihub, do you think this can cause issues, or do you think plugins will recover just fine when kubelet is alive again?

Dropping (connection establishment) timeout and the other changes were made based on the gRPC documentation, in particular the API functions we stopped using being marked as deprecated. That's why, for instance, we gave up using WithBlock().

I haven't checked the code but I think what happens is that an asynchronous connection establishment in the background is triggered by grpc.NewClient() and it returns immediately, without blocking. This was already the default behavior without the extra WithBlock() option. Then if a request is coming while connection establishment is still going on, the timeout for that request eventually kicks in if connection establishment is too slow.

There are potential annoying side-effects if the socket/RPC endpoint is optional, either there or not based on some config, but AFAICT that is not the case with the Pod Resource API socket. One real danger here is could be that if the initial connection establishment took way too long, then our very first API query could time out because of that but subsequent ones succeed... but if that starts showing up, we could do an extra List() or Get() as part of podresapi.NewClient() to make sure the connection is up.

@askervin askervin merged commit c52c7d4 into containers:main Dec 11, 2024
3 checks passed
@klihub klihub deleted the fixes/golangci-lint/agent branch December 11, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants